port TypeDescriptor benchmarks #83

adamsitnik · 2018-07-02T19:18:19Z

Fixes #48

This an excellent example why benchmarks MUST NOT contain any Asserts. I have ported the benchmarks and removed the asserts and BDN reported 1/7 of the time reported by xunit-performance.

I was shocked!

And then I used ILSpy to see what Assert was being used in the benchmark. The one from full xunit framework ;)

using (iteration.StartMeasurement())
    for (int i = 0; i < innerIterations; i++)
    {
        TypeConverter converter = TypeDescriptor.GetConverter(typeToConvert);
        Assert.NotNull(converter);
        Assert.Equal(expectedConverter, converter.GetType());
        Assert.True(converter.CanConvertTo(typeof(string)));
    }

So Asserts were 6/7 of the benchmarked time ;)

I did not have to port the Asserts to CoreFX because they are already there (each case has it's own test class)

Method	typeToConvert	expectedConverter	Mean	Error	StdDev	Median	Min	Max	Gen 0	Allocated
GetConverter	ClassIDerived	IBaseConverter	19.54 us	1.2412 us	1.4294 us	18.99 us	18.21 us	22.86 us	0.5833	3.91 KB
GetConverter	ClassWithNoConverter	TypeConverter	17.92 us	0.3542 us	0.3637 us	17.70 us	17.56 us	18.46 us	0.5910	3.91 KB
GetConverter	DerivedClass	DerivedClassConverter	26.54 us	1.6099 us	1.7894 us	25.61 us	24.33 us	30.00 us	0.5692	3.91 KB
GetConverter	IDerived	IBaseConverter	15.97 us	0.3967 us	0.4409 us	15.74 us	15.61 us	16.84 us	0.5934	3.91 KB
GetConverter	SomeEnum	EnumConverter	34.05 us	0.7000 us	0.7188 us	33.55 us	33.37 us	35.04 us	0.5896	3.91 KB
GetConverter	Enum	EnumConverter	27.20 us	0.6635 us	0.6813 us	26.97 us	26.82 us	29.44 us	0.5744	3.91 KB
GetConverter	Guid	GuidConverter	27.25 us	0.5298 us	0.5203 us	26.94 us	26.82 us	28.03 us	0.5000	3.91 KB
GetConverter	Int32	Int32Converter	27.22 us	0.5965 us	0.6383 us	26.90 us	26.72 us	28.86 us	0.6033	3.91 KB
GetConverter	SomeValueType?	NullableConverter	25.83 us	1.4700 us	1.6929 us	25.29 us	24.38 us	29.72 us	0.5379	3.91 KB
GetConverter	Int32?	NullableConverter	25.03 us	1.1559 us	1.2368 us	24.47 us	24.28 us	28.14 us	0.5521	3.91 KB
GetConverter	String	StringConverter	18.87 us	0.5419 us	0.5799 us	18.83 us	18.31 us	20.12 us	0.5704	3.91 KB

I also removed duplicated test cases, which were benchmarking same execiton path.

/cc @jorive

jorive · 2018-07-02T20:38:01Z

src/benchmarks/corefx/System.ComponentModel.TypeConverter/Perf.TypeDescriptorTests.cs

+        [Arguments(typeof(ClassIBase), typeof(IBaseConverter))]
+        [Arguments(typeof(ClassIDerived), typeof(IBaseConverter))]
+        [Arguments(typeof(Uri), typeof(UriTypeConverter))]
+        public TypeConverter GetConverter(Type typeToConvert, Type expectedConverter) // the expectedConverter argument is not used anymore, but kept to remain BenchView ID, do NOT remove


// [](start = 86, length = 2)

Should we remove this and start a fresh/clean benchmark? #Closed

You mean the expectedConverter argument?

Or in general?

In this case, the expectedConverter unused variable. #Closed

But, this would be a good opportunity to start a cleaner/newer benchmark too. #Closed

But, this would be a good opportunity to start a cleaner/newer benchmark too.

hmm, since after removing the Asserts from the benchmark the time is 1/7 of what it was before, I can remove this argument and rename the benchmark

@jorive I have removed the unused argument and removed the inner iteration count to let BDN do the job (it's a new benchmark after rename so I could do that)

# Conflicts: # src/benchmarks/Benchmarks.csproj

jorive

adamsitnik added 7 commits July 2, 2018 13:42

copy

c1acef1

port and remove the asserts

c3744f2

update to BDN which exports type arguments IDs as xunit does

1a5c4e7

add a comment about not removing expectedConverter argument

db672ec

update to BDN which shows simple, not-trimmed type names

0d203ae

reduce the number of test-cases, but still cover all paths

6a9337d

remove dead code

df74dfb

adamsitnik requested review from ViktorHofer and jorive July 2, 2018 19:18

jorive reviewed Jul 2, 2018

View reviewed changes

adamsitnik added 2 commits July 11, 2018 12:59

Merge remote-tracking branch 'upstream/master' into typeDescriptor

5f51c66

# Conflicts: # src/benchmarks/Benchmarks.csproj

change benchmark name and remove inner iterations to use BDN features

7d69ea7

jorive approved these changes Jul 11, 2018

View reviewed changes

adamsitnik merged commit 4196ee7 into dotnet:master Jul 11, 2018

adamsitnik deleted the typeDescriptor branch October 17, 2018 15:04

adamsitnik mentioned this pull request Mar 11, 2019

Switch to the dotnet/performance repo dotnet/corefx#35781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port TypeDescriptor benchmarks #83

port TypeDescriptor benchmarks #83

adamsitnik commented Jul 2, 2018

jorive Jul 2, 2018 •

edited

Loading

adamsitnik Jul 3, 2018

adamsitnik Jul 3, 2018

jorive Jul 3, 2018 •

edited

Loading

jorive Jul 3, 2018 •

edited

Loading

adamsitnik Jul 10, 2018

adamsitnik Jul 11, 2018

jorive left a comment

port TypeDescriptor benchmarks #83

port TypeDescriptor benchmarks #83

Conversation

adamsitnik commented Jul 2, 2018

jorive Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

adamsitnik Jul 3, 2018

Choose a reason for hiding this comment

adamsitnik Jul 3, 2018

Choose a reason for hiding this comment

jorive Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

jorive Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

adamsitnik Jul 10, 2018

Choose a reason for hiding this comment

adamsitnik Jul 11, 2018

Choose a reason for hiding this comment

jorive left a comment

Choose a reason for hiding this comment

jorive Jul 2, 2018 •

edited

Loading

jorive Jul 3, 2018 •

edited

Loading

jorive Jul 3, 2018 •

edited

Loading